Skip to content

[CALCITE-7531] Add to BasicSqlType constructor accepting precision, scale and nullability#4947

Merged
snuyanzin merged 1 commit into
apache:mainfrom
snuyanzin:calcite7531
May 20, 2026
Merged

[CALCITE-7531] Add to BasicSqlType constructor accepting precision, scale and nullability#4947
snuyanzin merged 1 commit into
apache:mainfrom
snuyanzin:calcite7531

Conversation

@snuyanzin
Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-7531

Changes Proposed

before Calcite 1.38 it was possible to set nullability with setter
now it is impossible, also it is impossible to pass it via constructor together with precision and scale.

so Flink's code stops working after attempting to bump Calcite.
The PR adds constructor to resolve it

here it is Flink code using this https://github.com/apache/flink/blob/1990310135edfec7021f77e64dbb976100458909/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/schema/TimeIndicatorRelDataType.scala#L30-L38

Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found it strange not having this API, especially since you actually need an instance of a typeSystem to change the nullability...

}

/**
* Constructs a type with precision/length and scale.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The javadoc on this constructor is identical to that on the previous constructor. And it doesn't describe what this constructor does.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worse than that, the change you have made isn't consistent with the commit message. Sort it out, please.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the feedback, addressed

Copy link
Copy Markdown
Contributor

@julianhyde julianhyde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 when javadoc is fixed

@snuyanzin snuyanzin force-pushed the calcite7531 branch 2 times, most recently from 887c020 to 9612c6d Compare May 16, 2026 09:27
@snuyanzin snuyanzin requested a review from julianhyde May 16, 2026 12:11
@snuyanzin
Copy link
Copy Markdown
Contributor Author

javadoc fixed
if there is no objection I'm going to merge it next day

@sonarqubecloud
Copy link
Copy Markdown

@snuyanzin
Copy link
Copy Markdown
Contributor Author

squashed and rebased

@snuyanzin snuyanzin merged commit 3816664 into apache:main May 20, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants